Decide admin status from Firestore, not from the token claim - #182

A demoted admin kept full admin power for up to an hour, and could use it to re-promote themselves permanently.

The chain

firestore.rules:30 returned true on a positive admin custom claim alone, as a cheap path with no Firestore read:

('admin' in request.auth.token && request.auth.token.admin == true) ||

functions/src/users.ts:245-250 — onAdminStateWritten clears that claim on demotion, but nothing revokes the already-issued id token. grep -rn revokeRefreshTokens src/ functions/src/ scripts/ functions/scripts/ is empty. Rules verify only a JWT's signature and expiry — not whether the claim inside it is still true, nor even that the Auth user still exists.

firestore.rules:89 put role in the client-writable allowlist, and firestore.rules:201-202 gated that document on isAdmin() alone.

So: the owner demotes an admin in the console. Within the next hour that person — a non-admin as far as the database is concerned — runs setDoc(doc(db,'users',<ownUid>,'admin','state'), {role:'admin'}, {merge:true}). The stale claim satisfies isAdmin(), role is allowlisted, the write lands, and onAdminStateWritten re-issues admin: true for real. The demotion is undone by the person demoted, and it stays undone.

Even without the re-promotion, that hour bought: banning any user, reading every feedback submitter's email (moderation.ts:147), deleting anyone's notifications, and writing arbitrary fields to any user document — the isAdmin() branch at firestore.rules:135 has no field allowlist.

The callable layer was never affected: getNotificationActor reads admin/state from Firestore, so caller.role !== "admin" was always current. This was rules-only.

Why the direction of trust was the bug

isNotBanned() trusts a positive banned claim too — and that's fine:

Not a caching problem. A direction problem.

The fix

isAdmin() reads admin/state — what the callables have always done, so the two layers now agree. exists() stays first so the helper returns false rather than raising for a user with no such document; an evaluation error would propagate through isAdmin() || (own record) and lock ordinary users out of their own reports and feedback. That's #150, and its test still passes.

Cost is one document read wherever isAdmin() is actually reached. It sits behind a short-circuit on every hot path — isOwner(userId) || isAdmin(), (isOwner && isAllowedUserUpdate()) || isAdmin() — so the read lands on admin surfaces and on reports/feedback reads by their own author. Low traffic. isNotBanned() already pays a comparable read on every like and bookmark.

Second change: no client write may introduce or change role, not even a real admin's. Promotion is an owner operation via the console or Admin SDK, both of which bypass rules, and the app has never written it — admin.ts:235-239, the only caller, writes the three ban fields. One compromised admin session can no longer mint more admins.

A trap I did not walk into

role deliberately stays in isAllowedAdminStateWrite's allowlist. That list says which keys may exist, and request.resource.data on an update is the whole post-write document, not the changed keys. Dropping role would not have stopped a client writing it — it would have stopped admins banning anyone the Admin SDK had already given one.

That is precisely the bug hasSafePublicLocation is stuck in today (audit finding 4: a legacy location.lat wedges every write to that user doc). Same shape, one file apart. So the restriction is expressed per-operation instead: absent on create, unaffected on update. There's a test pinning that banning a role-carrying user still works.

Considered and rejected

revokeRefreshTokens on demotion. It stops the client obtaining a new id token, but Firestore rules validate only signature and expiry, so the current one keeps satisfying them until it expires. It would have narrowed nothing that the document read doesn't close completely.

Tests

tests/rules/users.test.ts, 15 → 20 tests.

Three fail on the old rules and pass on these:

× refuses to let even a real admin write role from the client
× grants nothing to a token claiming admin with no admin/state doc
× does not let a demoted admin re-promote themselves on the stale claim

The third is the exploit end to end: seed admin/state = {role:'user'} (demoted), hand the context an admin: true claim, try to write role:'admin' back both ways.

Two more pass on both, on purpose — they're boundary guards, not the red signal:

• a freshly promoted admin whose token carries no claim at all is still recognised (the propagation-lag case the shortcut existed for — this is why isAdmin() reads the document rather than AND-ing it with the claim)
• banning a user whose document already carries a role still works

Full local run: rules 50/50, functions test:emulator 73/73, functions lint/build/typecheck:test clean, root typecheck:tests clean.

Deploy

Rules are the only part with runtime effect — the users.ts change is a comment correcting what it says about the claim/rules relationship.

firebase deploy --only firestore:rules

🤖 Generated with Claude Code